Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thumbs-up function 👍 #626

Merged
merged 45 commits into from
Feb 16, 2020
Merged

Thumbs-up function 👍 #626

merged 45 commits into from
Feb 16, 2020

Conversation

mkeeda
Copy link
Contributor

@mkeeda mkeeda commented Jan 26, 2020

Issue

Overview (Required)

  • Implement thumbs-up button like medium's crap
  • Thumbs-up counts are shared to all users
  • Only request to update the thumbs-up count if a particular timespan has passed

Links

Screenshot

Before After

@takahirom
Copy link
Member

👀

@takahirom
Copy link
Member

I commented in this 🙏
#429 (comment)

@takahirom
Copy link
Member

I applied some fixes. Can you check it? 🙏
4a2de25

@takahirom
Copy link
Member

image

@mkeeda
Copy link
Contributor Author

mkeeda commented Feb 2, 2020

@takahirom
Thank you for fix my code
I checked your code, but I caught the permission error too.
#626 (comment)
286d6bd debugging code is used by me.

Now, it is failure to retrieve shards and to create shards.
I cannot access to thumbsup_counters collection, probably.
Is my development enviroment some wrong?

I tried the simple code that retrieves documents of thumbsup_counters, but I cannot retrieve documents and catch the error.

code

FirebaseFirestore
    .getInstance()
    .collection("confsched/2020/sessions/155510/thumbsup_counters")
    .get()
    .addOnFailureListener {
        println(it)
    }
    .addOnSuccessListener {
        println(it.documents)
    }
error 2020-02-02 19:35:30.056 9913-9913/io.github.droidkaigi.confsched2020.debug E/AndroidRuntime: FATAL EXCEPTION: main Process: io.github.droidkaigi.confsched2020.debug, PID: 9913 com.google.android.gms.tasks.RuntimeExecutionException: com.google.firebase.firestore.FirebaseFirestoreException: PERMISSION_DENIED: Missing or insufficient permissions. at com.google.android.gms.tasks.zzu.getResult(Unknown Source:15) at io.github.droidkaigi.confsched2020.data.firestore.internal.FirestoreImpl$getFavoriteSessionIds$2.onComplete(FirestoreImpl.kt:54) at com.google.android.gms.tasks.zzj.run(Unknown Source:4) at android.os.Handler.handleCallback(Handler.java:883) at android.os.Handler.dispatchMessage(Handler.java:100) at android.os.Looper.loop(Looper.java:214) at android.app.ActivityThread.main(ActivityThread.java:7356) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930) Caused by: com.google.firebase.firestore.FirebaseFirestoreException: PERMISSION_DENIED: Missing or insufficient permissions. at com.google.firebase.firestore.util.Util.exceptionFromStatus(com.google.firebase:firebase-firestore@@20.2.0:121) at com.google.firebase.firestore.core.EventManager.onError(com.google.firebase:firebase-firestore@@20.2.0:134) at com.google.firebase.firestore.core.SyncEngine.handleRejectedListen(com.google.firebase:firebase-firestore@@20.2.0:399) at com.google.firebase.firestore.core.FirestoreClient.handleRejectedListen(com.google.firebase:firebase-firestore@@20.2.0:287) at com.google.firebase.firestore.remote.RemoteStore.processTargetError(com.google.firebase:firebase-firestore@@20.2.0:555) at com.google.firebase.firestore.remote.RemoteStore.handleWatchChange(com.google.firebase:firebase-firestore@@20.2.0:436) at com.google.firebase.firestore.remote.RemoteStore.access$100(com.google.firebase:firebase-firestore@@20.2.0:53) at com.google.firebase.firestore.remote.RemoteStore$1.onWatchChange(com.google.firebase:firebase-firestore@@20.2.0:176) at com.google.firebase.firestore.remote.WatchStream.onNext(com.google.firebase:firebase-firestore@@20.2.0:108) at com.google.firebase.firestore.remote.WatchStream.onNext(com.google.firebase:firebase-firestore@@20.2.0:38) at com.google.firebase.firestore.remote.AbstractStream$StreamObserver.lambda$onNext$1(com.google.firebase:firebase-firestore@@20.2.0:119) at com.google.firebase.firestore.remote.AbstractStream$StreamObserver$$Lambda$2.run(Unknown Source:4) at com.google.firebase.firestore.remote.AbstractStream$CloseGuardedRunner.run(com.google.firebase:firebase-firestore@@20.2.0:67) at com.google.firebase.firestore.remote.AbstractStream$StreamObserver.onNext(com.google.firebase:firebase-firestore@@20.2.0:110) at com.google.firebase.firestore.remote.FirestoreChannel$1.onMessage(com.google.firebase:firebase-firestore@@20.2.0:120) at io.grpc.ForwardingClientCallListener.onMessage(ForwardingClientCallListener.java:33) at io.grpc.ForwardingClientCallListener.onMessage(ForwardingClientCallListener.java:33) at io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl$1MessagesAvailable.runInContext(ClientCallImpl.java:563) at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37) at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:123) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:462) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) at com.google.firebase.firestore.util.AsyncQueue$DelayedStartFactory.run(com.google.firebase:firebase-firestore@@20.2.0:205) at java.lang.Thread.run(Thread.java:919) Caused by: io.grpc.StatusException: PERMISSION_DENIED: Missing or insufficient permissions. 2020-02-02 19:35:30.056 9913-9913/io.github.droidkaigi.confsched2020.debug E/AndroidRuntime: at io.grpc.Status.asException(Status.java:541) at com.google.firebase.firestore.util.Util.exceptionFromStatus(com.google.firebase:firebase-firestore@@20.2.0:119) ... 26 more

@takahirom
Copy link
Member

We haven't changed any rules since we created the issue.
Perhaps you can create that collection, but you don't have permission to get it, so it is expected that isEmpty will be true.
I am thinking about rule settings that can only get the size.

rules_version = '2';
service cloud.firestore {
  match /databases/{database}/documents {
    match /confsched/2020 {
...
      match /sessions/{sessionId} {
        match /{allSubcollections=**} {
          allow create
        }
        // Allow to increment only the 'shards' field and only by 1.
        match /thumbsup_counters/{counterId} {
          allow get;
          allow write: if request.resource.data.keys() == ["shards"]
                         && (resource == null || request.resource.data.shards ==
                         resource.data.shards + 1);
        }
      }
    }
  }
}

@takahirom
Copy link
Member

I changed the rule 🙏

rules_version = '2';
service cloud.firestore {
  match /databases/{database}/documents {
    match /confsched/2020 {
...
      match /sessions/{sessionId} {
        match /{allSubcollections=**} {
          allow create
        }
        match /thumbsup_counters {
          allow read
       	  // Allow to increment only the 'shards' field and only by 1.
          match /{counterId} {
            allow write: if request.resource.data.keys() == ["shards"]
                           && (resource == null || request.resource.data.shards ==
                           resource.data.shards + 1);
          }
        }
      }
    }
  }
}

@takahirom
Copy link
Member

I changed!

      match /sessions/{sessionId} {
        match /{allSubcollections=**} {
          allow create
        }
        match /thumbsup_counters/{counterId} {
            allow read;
        	  // Allow to increment only the 'shards' field and only by 1.
            allow write: if request.resource.data.keys() == ["shards"]
                           && (resource == null || request.resource.data.shards ==
                           resource.data.shards + 1);
        }
      }

@mkeeda mkeeda mentioned this pull request Feb 2, 2020
@mkeeda
Copy link
Contributor Author

mkeeda commented Feb 2, 2020

The code fail to create shards...
I misunderstood that I successed to create shards...?
I will find out the cause tomorrow.

@takahirom
Copy link
Member

Maybe even if there is a value, I think that isEmpty is true.
Personally, I think it's a good idea to check if there are 9 counters so that the process can die during creation.
Or it might be better to use something like a transaction
https://firebase.google.com/docs/firestore/manage-data/transactions

@@ -10,6 +10,7 @@
<item name="textAppearanceBody1">@style/TextAppearance.DroidKaigi.Body1</item>
<item name="textAppearanceBody2">@style/TextAppearance.DroidKaigi.Body2</item>
<item name="textAppearanceCaption">@style/TextAppearance.DroidKaigi.Caption</item>
<item name="textAppearanceButton">@style/TextAppearance.DroidKaigi.Button</item>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@takahirom
Copy link
Member

I changed the rule 🙏

                           request.resource.data.shards > resource.data.shards &&
                           request.resource.data.shards < resource.data.shards + 51


// This function is copied from https://medium.com/androiddevelopers/suspending-over-views-19de9ebd7020

suspend fun Animator.awaitEnd() = suspendCancellableCoroutine<Unit> { cont ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

)
}

val appError: LiveData<AppError?> = merge(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question. Please tell me why you separated the error 🙋‍♂
for simplifying??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
The reason is that uiModel's errors is complexed.
In my opinion, it is easy to read that the code is separated success and failure flow.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have added the errorGettable interface. 👍
I think that you can write enough with UiModel by using this. 👀
70f7d25

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, besides whether it is good or bad, I would like to challenge this DroidKaigi to make one UiModel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's challenge the one UiModel!
70f7d25 is enough to simple too for me.

@takahirom takahirom added this to the 1.1.0 milestone Feb 15, 2020
@takahirom
Copy link
Member

@mkeeda
Thanks as always 🙏
Can you take time today?
What are the remaining tasks now?

@mkeeda
Copy link
Contributor Author

mkeeda commented Feb 16, 2020

@takahirom
Sorry, I’m almost done!
I will finish tasks today.
The remaining tasks ↓

  • Cherry-pick 70f7d25 (Conflicting my changes...)
  • Refactor animation functions
  • Remove debug log (I want to remove ⭐print)

I will write overviews at top of this PR when I finished all tasks.

@takahirom
Copy link
Member

Thanks! I want to merge this today!! 👍

@takahirom
Copy link
Member

Can you delete WIP? 😄

@mkeeda
Copy link
Contributor Author

mkeeda commented Feb 16, 2020

Yes!
I'm writing overviews now!

@mkeeda mkeeda changed the title [WIP] Thumbs-up function Thumbs-up function 👍 Feb 16, 2020
@jmatsu-bot
Copy link
Collaborator

Your apk has been deployed to https://deploygate.com/distributions/1f10badbf5012479d46a7dffcc44c18b5a4c3beb. Anyone can try your changes via the link.

Generated by 🚫 Danger

@jmatsu-bot
Copy link
Collaborator

No error was reported but at least one warning was found.

Generated by 🚫 Danger

@mkeeda
Copy link
Contributor Author

mkeeda commented Feb 16, 2020

@takahirom
All tasks is done!
Please review again 🙏 🙏 🙏

@takahirom
Copy link
Member

LGTM 👍👍👍👍

@takahirom takahirom merged commit 413143f into DroidKaigi:master Feb 16, 2020
@mkeeda mkeeda deleted the thumb-up branch February 17, 2020 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thumbs-up function
3 participants